Make ChangeDestinationSourceSyncWrapper pub
#3787
Conversation
The idea of the wrapper is that you'd use it to wrap your `ChangeDestinationSourceSync` instance. To be able to do that outside of `lightning`, we'll need to make it `pub`.
|
👋 Thanks for assigning @joostjager as a reviewer! |
| @@ -996,12 +996,7 @@ pub trait ChangeDestinationSourceSync { | |||
| } | |||
|
|
|||
| /// A wrapper around [`ChangeDestinationSource`] to allow for async calls. | |||
| #[cfg(any(test, feature = "_test_utils"))] | |||
| pub struct ChangeDestinationSourceSyncWrapper<T: Deref>(T) | |||
There was a problem hiding this comment.
Do you still need this public when using OutputSweeperSync which does the wrapping for you?
There was a problem hiding this comment.
Currently yes: in LDK node we use (and like to continue using) the async BackgroundProcessor, which however only works with OutputSweeper, not OutputSweeperSync. So we need to wrap ourselves, see this commit: lightningdevkit/ldk-node@bd46d1f
There was a problem hiding this comment.
Hmm if we do this, isn't this going to risk blocking tokio?
There was a problem hiding this comment.
Ugh, you're right. Not in LDK Node (as we know the implementation is sync), but in general yes, this would open us up to the footgun we discussed previously. Kinda telling that I already almost forgot... Maybe the ChangeDestinationSourceSync could use some additional docs reminding us that we can only ever implement it via OutputSweeperSync?
I guess I'll need to do the ugly thing then and use spawn_blocking in the async ChangeDestinationSource.
There was a problem hiding this comment.
Maybe the ChangeDestinationSourceSync could use some additional docs reminding us that we can only ever implement it via OutputSweeperSync
Yes can add that.
I guess I'll need to do the ugly thing then and use spawn_blocking in the async ChangeDestinationSource.
That is indeed the consequence of calling the sweeper from an async context. Isn't it possible to create a native async ChangeDestinationSource in ldk-node?
There was a problem hiding this comment.
Isn't it possible to create a native async ChangeDestinationSource in ldk-node?
It is, should have gone that way from the beginning, even though it entailed some annoying refactoring: lightningdevkit/ldk-node@c7b74640
There was a problem hiding this comment.
Looks good. This is what we always wanted, isn't it?
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
The idea of the wrapper is that you'd use it to wrap your
ChangeDestinationSourceSyncinstance. To be able to do that outside oflightning, we'll need to make itpub.